Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Revert "path: resolve normalize drive letter to lower case" #100

Closed
wants to merge 1 commit into from
Closed

Revert "path: resolve normalize drive letter to lower case" #100

wants to merge 1 commit into from

Conversation

seishun
Copy link
Contributor

@seishun seishun commented Dec 6, 2014

This fixes realpath, but test-fs-realpath still fails due to a different issue. Looking into it.

See #17.

@bnoordhuis
Copy link
Member

The commit log should explain why it reverts f6e5740, i.e. why the change in that commit is wrong.

@bnoordhuis
Copy link
Member

@piscisaureus Can you take a look?

@seishun
Revert "path: resolve normalize drive letter to lower case"
87cb68a 
This reverts commit f6e5740.

Changing drive letters to lowercase violates the principle of
least surprise. Other functions that do this should get fixed too.

Conflicts:
	lib/path.js
@seishun
Copy link
Contributor Author

seishun commented Dec 6, 2014

I'm not sure how much of the backstory is should include. I've added a short rationale for now.

@bnoordhuis
Copy link
Member

Thanks, @seishun.

@piscisaureus PTAL. If this was discussed somewhere (which I believe it was), can you add a link?

@rvagg
Copy link
Member

rvagg commented Dec 6, 2014

@seishun do the tests enforce this behaviour or do we need a new explicit test to lock it in place?

@rvagg rvagg mentioned this pull request Dec 6, 2014
Closed
@seishun
Copy link
Contributor Author

seishun commented Dec 6, 2014

@rvagg the tests are already case-sensitive, I don't think they could do any better.

By the way, the test still fails with this change due to path.join() changing the case of the drive letter (see nodejs/node-v0.x-archive#7031) due to the path.normalize() change mentioned in node-forward/node#20. Should I go ahead and submit a PR fixing that?

@caineio
Copy link

caineio commented Dec 8, 2014

Hello!

I am pleased to see your valuable contribution to this project. Would you
please mind answering a couple of questions to help me classify this submission
and/or gather required information for the core team members?

Questions:

  1. Which part of core do you think it might be related to?
    One of: debugger, http, assert, buffer, child_process, cluster, crypto, dgram, dns, domain, events, fs, http, https, module, net, os, path, querystring, readline, repl, smalloc, stream, timers, tls, url, util, vm, zlib, c++, docs, other (label)
  2. Which versions of io.js do you think are affected by this?
    One of: v0.10, v0.12, v1.0.0 (label)
  3. PR-only Does make test pass after applying this Pull Request.
    Expected: yes
  4. PR-only Is the commit message properly formatted? (See
    CONTRIBUTING.md for more information)
    Expected: yes

Please provide the answers in an ordered list like this:

  1. Answer for the first question
  2. Answer for the second question
  3. ...

Note that I am just a bot with a limited human-reply parsing abilities,
so please be very careful with numbers and don't skip the questions!

In case of success I will say: ...summoning the core team devs!.

In case of validation problem I will say: Sorry, but something is not right here:.

Truly yours,
Caine.

Responsibilities

  1. indutny: crypto, tls, https, child_process, c++
  2. trevnorris: buffer, http, https, smalloc
  3. bnoordhuis: http, cluster, child_process, dgram

piscisaureus pushed a commit that referenced this pull request Dec 8, 2014
@seishun @piscisaureus
Revert "path: resolve normalize drive letter to lower case"
e24fa83 
This reverts commit f6e5740.

Changing drive letters to lowercase violates the principle of
least surprise. Other functions that do this should get fixed too.

Conflicts:
	lib/path.js

PR-URL: #100
Reviewed-By: Bert Belder <bertbelder@gmail.com>
@piscisaureus piscisaureus mentioned this pull request Dec 8, 2014
Closed
@piscisaureus piscisaureus self-assigned this Dec 8, 2014
@piscisaureus piscisaureus mentioned this pull request Dec 8, 2014
Closed
piscisaureus pushed a commit to piscisaureus/node that referenced this pull request Dec 23, 2014
@seishun @piscisaureus
path: don't lower-cases drive letters
2bec35e 
Changing drive letters to lowercase violates the principle of
least surprise.

PR-URL: nodejs/node#100
piscisaureus added a commit to piscisaureus/node that referenced this pull request Dec 23, 2014
@piscisaureus
path: don't lower-cases drive letters
f09e321 
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

PR-URL: nodejs/node#100
@rvagg rvagg mentioned this pull request Jan 3, 2015
Closed
misterdjules pushed a commit to misterdjules/node that referenced this pull request Jan 15, 2015
@piscisaureus
path: don't lower-cases drive letters
2978f30 
In general path functions don't change the case of a path. Making an
exception for windows drive letters violates the principle of least
surprise.

Changing the drive letter case has caused a lot of issues, including
nodejs#7031, nodejs#7806 and lots of bikeshedding about
whether uppercase is the right case or lowercase.

This effectively reverts nodejs/node-v0.x-archive@a05f973

PR-URL: nodejs/node#100
@zcbenz zcbenz mentioned this pull request Mar 25, 2015
Closed
@mrvini mrvini mentioned this pull request Jun 6, 2016
Closed
@weekens weekens mentioned this pull request Jun 17, 2017
Closed
@ystskm ystskm mentioned this pull request Jun 19, 2017
Closed
@mohanen mohanen mentioned this pull request Aug 28, 2017
Closed
@codebytere codebytere mentioned this pull request May 7, 2020
Closed
@wmertens wmertens mentioned this pull request Jun 19, 2020
Closed
@cemerick cemerick mentioned this pull request Nov 19, 2021
Open
@cemerick cemerick mentioned this pull request Nov 28, 2021
Closed
@snyk-bot snyk-bot mentioned this pull request Dec 21, 2021
Open
@jgowdy jgowdy mentioned this pull request Apr 7, 2022
Open
@pratapadi pratapadi mentioned this pull request Aug 11, 2022
Open
@verdinjoshua1982 verdinjoshua1982 mentioned this pull request Oct 14, 2022
Open
@dubzzz dubzzz mentioned this pull request Dec 26, 2022
Closed
This was referenced Jul 6, 2023
Open
Open
Merged
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
Open
@dewaynesmith3711 dewaynesmith3711 mentioned this pull request Nov 30, 2023
Open
@ZRDK ZRDK mentioned this pull request Nov 30, 2023
Open
@karencapiiro karencapiiro mentioned this pull request Nov 30, 2023
Open
@BryanLaura BryanLaura mentioned this pull request Nov 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@seishun @bnoordhuis @rvagg @caineio @piscisaureus